Start phasing out #[cfg(debug_assertions)] in favor of safeguards
#1395
Conversation
Bromeon
added
the
quality-of-life
|
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1395 |
Bromeon
force-pushed
the
qol/safeguard-assertions
branch
2 times, most recently
from
93e94b3 to
7741e64
Compare
| rust_function, | ||
| #[cfg(debug_assertions)] | ||
| Self::from_fn_wrapper( | ||
| name, |
There was a problem hiding this comment.
Function docs must be updated:
nameis used for the string representation of the closure, which helps with debugging.
While putting it under safeguards makes sense, I would expect debugging info to be controlled by debug/release mode.
Same in other places
There was a problem hiding this comment.
While putting it under safeguards makes sense, I would expect debugging info to be controlled by debug/release mode.
Strictly speaking there is no Debug/Release mode, only debug_assertions being on/off -- but yes, this typically corresponds to Rust's dev/release profiles, unless changed. However, in practice it's more "running in Godot editor or release-template", which is a runtime decision.
Generally I'd like to move away from debug_assertions, because safeguards are more flexible. This particular change was also motivated by performance alone: #1331
It might be an option to make the name still available in all safeguard levels, by simply storing it as Cow<str> or so, and access it only when needed (e.g. to_string). This could avoid a lot of allocations and conversions, because most users likely use literals here. Such a change would be out-of-scope for this PR though.
There was a problem hiding this comment.
I do agree, that's why I mentioned that it makes sense – I would only update function docs, to inform when name is being optimized out 😅.
There was a problem hiding this comment.
I'll create a follow-up PR and do it there, maybe we can avoid optimizing it out 🙂
| /// # Panics | ||
| /// In Debug mode, if `index` is out of bounds. In Release, a Godot error is generated and the result is unspecified (but safe). | ||
| /// # Panics (safeguards-balanced) | ||
| /// If `index` is out of bounds. In safeguards-disengaged mode, a Godot error is generated and the result is unspecified (but safe). |
There was a problem hiding this comment.
Not really related to this PR but what does "safe" mean in this context 🤔?
There was a problem hiding this comment.
Cannot cause UB. The behavior is unspecified.
This is explicitly mentioned because in many scenarios, absent validations in safeguards-disengaged immediately cause undefined behavior.
There was a problem hiding this comment.
I wondered if we shouldn't call it "sound" but safe is more straightforward and easier to grasp, I think 🤔
There was a problem hiding this comment.
Might be, although "safe" implies "sound".
- Safe: there can be no UB.
- Unsafe: it's the user's responsibility to not cause UB.
- Unsound: there is UB, something needs to be fixed.
| /// # Panics | ||
| /// In Debug mode, if `index` is out of bounds. In Release, a Godot error is generated and the result is unspecified (but safe). | ||
| /// # Panics (safeguards-balanced) | ||
| /// If `index` is out of bounds. In safeguards-disengaged mode, a Godot error is generated and the result is unspecified (but safe). |
There was a problem hiding this comment.
safeguards-disengaged mode
level
| /// | ||
| /// # Panics | ||
| /// In debug mode, when from > to. | ||
| pub(crate) fn to_godot_range_fromto(range: impl SignedRange) -> (i64, i64) { |
There was a problem hiding this comment.
description must be updated too
/// # Panics (safeguards-strict)
/// When `from` > `to`.
| } | ||
| // Check that we're on the main thread. Only enabled with balanced+ safeguards and in multi-threaded builds. | ||
| // In wasm_nothreads, there's only one thread, so no check is needed. | ||
| #[cfg(all(safeguards_balanced, not(wasm_nothreads)))] |
There was a problem hiding this comment.
I wonder if early return with if cfg!("wasm_nothreads") { return } wouldn't be more readable 🤔
There was a problem hiding this comment.
Added a ensure_main_thread() function to extract it.
- Early return doesn't gain much because we'd still have to cover
#[cfg(...)]across both the return and the remaining logic. - The alternative is having 2 functions (one no-op), but that also needs two
#[cfg(...)]and much more LoC + noise. - Easiest might be
cfg!but then I'm not sure if it's fully optimized away, in this method is a bit risky.
Bromeon
force-pushed
the
qol/safeguard-assertions
branch
from
7741e64 to
5e75d1c
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
3 participants
Following up #1278, this PR introduces two new assertion groups:
sys::strict_assert!-- validate in strict level onlysys::balanced_assert!-- validate in strict+balanced levelsThis replaces a majority of
debug_asserts in the library. The idea is to slowly move away from them.Regular
assert!will still be used, whenever all levels need to be validated, concretely:The PR should not introduce major semantic changes, however it's very likely that conditions previously not validated are validated now, and vice versa, depending on your level.
More work is planned in the future, like turning off more checks in disengaged level.